Adds toast ID to toast api #3752
Conversation
Signed-off-by: Ashwin P Chandran <[email protected]>
Signed-off-by: Ashwin P Chandran <[email protected]>
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #3752 +/- ##
==========================================
+ Coverage 66.43% 66.45% +0.01%
==========================================
Files 3209 3208 -1
Lines 61741 61752 +11
Branches 9537 9539 +2
==========================================
+ Hits 41020 41035 +15
+ Misses 18431 18429 -2
+ Partials 2290 2288 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -42,12 +42,10 @@ import { I18nStart } from '../../i18n'; | |||
| /** | |||
| * Allowed fields for {@link ToastInput}. | |||
| * | |||
| * @remarks | |||
| * `id` cannot be specified. | |||
There was a problem hiding this comment.
do we know the historical reason why?
There was a problem hiding this comment.
I looked at the git history but as far as I can tell, there isn't a good reason. That's why I kept the default behavior as is. Only if you explicitly pass an id does the behavior change
There was a problem hiding this comment.
After reviewing, my understanding is that it was just intended to be an internal property, that simply auto-increments with each new toast. Ashwin's change here changes the purpose and usage of the property a bit (for example, passing string ids instead of a number), but not in a way that affects the historical usage. So I don't think it was intended as "specifying id is prohibited" and instead meant "specifying id is not necessary".
|
@ashwin-pc I tried re-running the cypress tests, but you may want to take a look at why there are failed tests there. |
ashwin-pc
requested review from
ananzh,
seanneumann,
AMoo-Miki,
joshuarrrr,
abbyhu2000,
zengyan-amazon,
kristenTian,
zhongnansu and
manasvinibs
as code owners
|
The branch was behind main by quite a bit. They should be passing now |
| return existingToast; | ||
| } | ||
| } | ||
|
|
||
| const toast: Toast = { | ||
| id: String(this.idCounter++), |
There was a problem hiding this comment.
I wonder if it's somewhat surprising that we still increment the idCounter, but throw away the value. On the one hand, it means that the counter really does match the total number, but on the other, there are now some idCounter values that don't exist in the map of toast IDs.
There was a problem hiding this comment.
We dont still increment, if an existing toast is found, we exit this function when we return the existing toast.
There was a problem hiding this comment.
Right, I'm talking about the case where an id is provided, but there's no existing matching toast. In that case we increment, but don't use the incremented value.
src/plugins/vis_builder/public/application/utils/state_management/store.ts
Show resolved
Hide resolved
Signed-off-by: Ashwin P Chandran <[email protected]> Co-authored-by: Josh Romero <[email protected]>
* Currently sending the same toast multiple times results in multiple toasts being rendered on the screen. This change allows the toast api to additionally accept an id parameter that * Update changelog * Update src/core/public/notifications/toasts/toasts_api.tsx Issue Resolved: #2643 Signed-off-by: Ashwin P Chandran <[email protected]> Co-authored-by: Josh Romero <[email protected]> --------- Signed-off-by: Ashwin P Chandran <[email protected]> Co-authored-by: Sean Neumann <[email protected]> Co-authored-by: Josh Romero <[email protected]> (cherry picked from commit 14bde2b) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
* Adds toast ID to toast api (#3752) * Currently sending the same toast multiple times results in multiple toasts being rendered on the screen. This change allows the toast api to additionally accept an id parameter that * Update changelog * Update src/core/public/notifications/toasts/toasts_api.tsx Issue Resolved: #2643 Signed-off-by: Ashwin P Chandran <[email protected]> Co-authored-by: Josh Romero <[email protected]> --------- Signed-off-by: Ashwin P Chandran <[email protected]> Co-authored-by: Sean Neumann <[email protected]> Co-authored-by: Josh Romero <[email protected]> (cherry picked from commit 14bde2b) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md * add changelog Signed-off-by: Josh Romero <[email protected]> --------- Signed-off-by: Josh Romero <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Josh Romero <[email protected]> Co-authored-by: Anan Zhuang <[email protected]>
Description
Currently sending the same toast multiple times results in multiple toasts being rendered on the screen. This change allows the toast api to additionally accept an id parameter that
Before:
Now:
Issues Resolved
Check List
yarn test:jestyarn test:jest_integrationyarn test:ftr